32943-Sugar for HierarchyId path generation #33062
Conversation
|
@dotnet-policy-service agree |
| /// <returns>A <see cref="HierarchyId" /> value.</returns> | ||
| [return: NotNullIfNotNull(nameof(parentHierarchyId))] | ||
| [return: NotNullIfNotNull(nameof(parentId))] | ||
| public static HierarchyId? Parse(HierarchyId parentHierarchyId , IReadOnlyList<int> parentId) |
There was a problem hiding this comment.
Why are nulls allowed here? Also, consider making this a TryAdd if it expected to fail, but I'm not sure it should fail, since even if the HierarchyId created is not valid, that won't be known until it is saved to the database.
There was a problem hiding this comment.
I understand what you mean; you are correct. I have fixed it in the This Commit.
| //This Method can move to "SqlHierarchyId in Microsoft.SqlServer.Types", if we don't want put it in this abstraction. | ||
| private static HierarchyId GenerateHierarchyIdBasedOnParent(HierarchyId parent, IReadOnlyList<int> parentId) | ||
| { | ||
| if (parent is null) |
There was a problem hiding this comment.
Add curlies here. In general, try to follow the formatting of other cs files in the project.
There was a problem hiding this comment.
Resolved in This Commit.
| /// </summary> | ||
| /// <param name="parentHierarchyId">The parent HierarchyId of node.</param> | ||
| /// <param name="parentId">The parent Id of current node. It can be more than one element if want have path like: "/1/2/3.1/", otherwise one element for have path like: "/1/2/3/".</param> | ||
| public HierarchyId(HierarchyId parentHierarchyId, IReadOnlyList<int> parentId) |
There was a problem hiding this comment.
Is there a need for both a Parse method and a constructor?
There was a problem hiding this comment.
I added it because another overload of the Parse Method is used in the constructor, and I think it's not a bad idea to include it. It can be removed and I removed the constructor with this overload in This Commit
…load' into 32943-add-hierarchyId-parse-overload
|
I attempted to simplify the new |
| specificPath.Append(string.Join(".", parentId)); | ||
| specificPath.Append('/'); | ||
|
|
||
| return HierarchyId.Parse(specificPath.ToString()); |
There was a problem hiding this comment.
I don't think this can ever return null, right? In which case the return type here should be non-nullable.
There was a problem hiding this comment.
If you mean this method will never have a null output, it is correct. I Modified the new Parse overload output type in This commit
Also, the main overload of the Parse method that used, will not have a null output if it has a non-null input.
[return: NotNullIfNotNull(nameof(input))]
public static HierarchyId? Parse(string? input)
=> (HierarchyId?)SqlHierarchyId.Parse(input);
|
Thanks for the contribution! |
Fixes #32943